-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address reuse improvements and fixes #3475
Conversation
6992266
to
2049ee7
Compare
@bors r+ |
when an address gets reused, establish a happens-before link in the data race model Fixes #3450
Ah... this makes the weak memory emulation basically disappear. We do so much reuse even of stack variables that it is almost guaranteed that you pick up some address from another thread, and therefore synchronize with it... |
…i-obk interpret: pass MemoryKind to before_memory_deallocation This will be needed for rust-lang/miri#3475. r? `@oli-obk`
…i-obk interpret: pass MemoryKind to before_memory_deallocation This will be needed for rust-lang/miri#3475. r? ``@oli-obk``
💥 Test timed out |
2049ee7
to
aa7f3a4
Compare
Rollup merge of rust-lang#124018 - RalfJung:dealloc-memory-kind, r=oli-obk interpret: pass MemoryKind to before_memory_deallocation This will be needed for rust-lang/miri#3475. r? ``@oli-obk``
interpret: pass MemoryKind to before_memory_deallocation This will be needed for #3475. r? ``@oli-obk``
…, r=oli-obk interpret: pass MemoryKind to adjust_alloc_base_pointer Another puzzle piece for rust-lang/miri#3475. The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address. rust-lang#124018 has been rolled up already so I couldn't add it there any more. r? `@oli-obk`
…, r=oli-obk interpret: pass MemoryKind to adjust_alloc_base_pointer Another puzzle piece for rust-lang/miri#3475. The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address. rust-lang#124018 has been rolled up already so I couldn't add it there any more. r? ``@oli-obk``
…, r=oli-obk interpret: pass MemoryKind to adjust_alloc_base_pointer Another puzzle piece for rust-lang/miri#3475. The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address. rust-lang#124018 has been rolled up already so I couldn't add it there any more. r? ``@oli-obk``
…, r=oli-obk interpret: pass MemoryKind to adjust_alloc_base_pointer Another puzzle piece for rust-lang/miri#3475. The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address. rust-lang#124018 has been rolled up already so I couldn't add it there any more. r? ```@oli-obk```
Rollup merge of rust-lang#124030 - RalfJung:adjust_alloc_base_pointer, r=oli-obk interpret: pass MemoryKind to adjust_alloc_base_pointer Another puzzle piece for rust-lang/miri#3475. The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address. rust-lang#124018 has been rolled up already so I couldn't add it there any more. r? ```@oli-obk```
interpret: pass MemoryKind to adjust_alloc_base_pointer Another puzzle piece for rust-lang#3475. The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address. rust-lang/rust#124018 has been rolled up already so I couldn't add it there any more. r? ```@oli-obk```
aa7f3a4
to
7a2251c
Compare
7a2251c
to
cb4f65b
Compare
cb4f65b
to
f3b31c3
Compare
I excluded stack allocations from reuse, and made the reuse rate configurable. @bors r+ |
Address reuse improvements and fixes - when an address gets reused, establish a happens-before link in the data race model - do not reuse stack addresses, and make the reuse rate configurable Fixes #3450
This actually still has a quite significant impact on data race detection; I suspect this is due to heap allocations in the thread spawning logic. I'm not sure what the best way forward is here... other than reducing the reuse rate drastically. With a reuse rate of 25%, the simple data race I checked is still found around 70% of the time (but it's 100% without reuse). With the default reuse rate it's about 40% detection rate for data races. I think the rate will be better with real programs; the issue is that in these tests the threads exist only for a very short time so there's a real chance it never gets preempted and so the first thread is done before the second thread starts, and therefore allocations can be reused. If I add a simple We could of course also just not do any reuse across threads, then we also will never add spurious synchronization. But for heap allocations that's not real either. |
💔 Test failed - checks-actions |
acbcc7a
to
34c8db6
Compare
OTOH, real programs will also do a bunch of stuff on the heap, which again increases the chance of address reuse inducting synchronization. My current inclination is to add some extra randomness that makes address reuse across thread boundaries a lot less likely than address reuse within a single thread. |
34c8db6
to
1582636
Compare
That's what I now implemented with the last commit. @rust-lang/miri what do you think? With the cross-thread reuse rate of 10%, the simple data race tests still detect the race 80% of the time when preemption is disabled (and ~100% of the time when preemption is enabled). These tests do basically nothing in their threads, so there's a significant chance the 2nd thread starts after the 1st thread is already done and then address reuse inside I'm thinking maybe I should increase the regular address reuse rate to around 70%; that makes the chance that a |
1582636
to
2155a30
Compare
That seems fine to me. Anyone seriously looking to find data races should have preemption enabled. |
Yeah... though it's hard to say what this will do for other cases. Not sure how many people run concurrency tests in a loop in Miri to give races more than one chance to appear. But for now, let's go ahead with this, we can always re-adjust later if we realize that data race detection has become too non-deterministic. @bors r+ |
☀️ Test successful - checks-actions |
Fixes #3450